Allow altering only either CPU or memory during VM live scale by GaOrtiga · Pull Request #8234 · apache/cloudstack

GaOrtiga · 2023-11-14T16:24:15Z

Description

The API scaleVirtualMachine requires the specification of both vCPU quantity and memory amount, resulting in an error if either was omitted. This means that if an operator wants to increase the quantity of vCPUs while maintaining the existing memory, they were obligate to provide the current memory value as well.

An alteration has been introduced to the API to allow operators to input only the parameters they intend to modify while retaining the current values for the rest.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

How Has This Been Tested?

I applied the changes in a local lab and verified that if I used the API while inputting only one of the parameters, the scale was successful, and the others kept the current value.

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

DaanHoogland

agree with @hsato03 , code lgtm otherwise

codecov · 2023-11-15T08:36:56Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (399bd0a) 30.90% compared to head (17f64f4) 30.74%.
Report is 19 commits behind head on main.

Files	Patch %	Lines
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java	92.85%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8234      +/-   ##
============================================
- Coverage     30.90%   30.74%   -0.17%     
+ Complexity    34142    33040    -1102     
============================================
  Files          5353     5352       -1     
  Lines        375900   374473    -1427     
  Branches      54629    54610      -19     
============================================
- Hits         116158   115114    -1044     
+ Misses       244428   244096     -332     
+ Partials      15314    15263      -51

Flag	Coverage Δ
simulator-marvin-tests	`24.58% <64.28%> (-0.13%)`	⬇️
uitests	`4.38% <ø> (-0.01%)`	⬇️
unit-tests	`16.41% <78.57%> (-0.21%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

DaanHoogland · 2023-11-16T11:57:17Z

@blueorangutan package

blueorangutan · 2023-11-16T11:58:03Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-11-16T12:56:53Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7771

DaanHoogland · 2023-11-16T12:58:35Z

@blueorangutan test

blueorangutan · 2023-11-16T13:00:04Z

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

blueorangutan · 2023-11-17T02:16:49Z

[SF] Trillian test result (tid-8336)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 46309 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8234-t8336-kvm-centos7.zip
Smoke tests completed. 116 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_02_upgrade_kubernetes_cluster	`Failure`	565.05	test_kubernetes_clusters.py
test_08_migrate_vm	`Error`	0.07	test_vm_life_cycle.py

JoaoJandre

CLGTM, didn't test it

GutoVeronezi · 2023-11-28T21:42:56Z

@hsato03, are all your concerns met?

Just for the sake: @blueorangutan package

blueorangutan · 2023-11-28T21:44:04Z

@GutoVeronezi a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

hsato03

@hsato03, are all your concerns met?

Just for the sake: @blueorangutan package

Yes, didn't test but CLGTM.

blueorangutan · 2023-11-28T22:44:43Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7859

shwstppr · 2023-12-06T11:17:14Z

@borisstoyanov are you doing tests on this?

github-actions · 2023-12-07T07:37:40Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

shwstppr · 2023-12-08T06:43:31Z

@blueorangutan package

blueorangutan · 2023-12-08T06:44:03Z

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-12-08T07:56:30Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7981

DaanHoogland · 2023-12-08T13:59:22Z

@blueorangutan test

blueorangutan · 2023-12-08T14:00:03Z

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2023-12-09T02:41:06Z

[SF] Trillian test result (tid-8525)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 44045 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8234-t8525-kvm-centos7.zip
Smoke tests completed. 120 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_02_list_cpvm_vm	`Failure`	0.03	test_ssvm.py
test_04_cpvm_internals	`Failure`	0.03	test_ssvm.py

github-actions · 2024-02-05T08:52:56Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

Co-authored-by: sato03 <henriquesato2003@gmail.com>

DaanHoogland · 2024-02-06T17:07:48Z

@blueorangutan package

blueorangutan · 2024-02-06T17:08:03Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-02-06T18:11:52Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8552

DaanHoogland · 2024-02-07T15:17:54Z

@blueorangutan test

blueorangutan · 2024-02-07T15:20:03Z

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2024-02-08T07:30:52Z

[SF] Trillian test result (tid-9123)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 56752 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8234-t9123-kvm-centos7.zip
Smoke tests completed. 122 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

DaanHoogland · 2024-02-09T16:01:35Z

@GaOrtiga this had already passed but needs new attention since the log upgrade

Co-authored-by: João Jandre <48719461+JoaoJandre@users.noreply.github.com>

DaanHoogland · 2024-02-12T10:45:04Z

@blueorangutan package

blueorangutan · 2024-02-12T10:46:03Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-02-12T11:46:02Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8629

yadvr · 2024-02-13T05:47:49Z

@blueorangutan test

blueorangutan · 2024-02-13T05:48:03Z

@rohityadavcloud a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2024-02-13T19:24:14Z

[SF] Trillian test result (tid-9186)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 47474 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8234-t9186-kvm-centos7.zip
Smoke tests completed. 129 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

GutoVeronezi · 2024-02-14T21:57:42Z

Merging based on approvals, packaging and test results.

DaanHoogland · 2024-02-15T09:34:36Z

As this is merged into main, I am setting the milestone to 20

weizhouapache · 2024-02-15T09:39:47Z

@DaanHoogland @GutoVeronezi
has anyone tested this PR ?

weizhouapache · 2024-02-16T20:19:28Z

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

+
+        updateInstanceDetails(cmdDetails, vm, newServiceOfferingId);
+
+        boolean result = upgradeVirtualMachine(vmId, newServiceOfferingId, cmdDetails);


Sorry for late review.

@GaOrtiga
if the upgrade fail, should the details be restored to the original values ?

ping @DaanHoogland @GutoVeronezi , anyone has tested it ?

ping @DaanHoogland @GutoVeronezi , anyone has tested it ?

Yes, it was tested.

if the upgrade fail, should the details be restored to the original values ?

The PR does not address changes to this behavior. it remains the same; we can discuss it in an issue or another PR.

ping @DaanHoogland @GutoVeronezi , anyone has tested it ?

Yes, it was tested.

Ok, I asked it because I did not see a comment mentioning it has been manually tested ok. If not, it would be good to be manually tested.
Why has tested it, except the author?

if the upgrade fail, should the details be restored to the original values ?

The PR does not address changes to this behavior. it remains the same; we can discuss it in an issue or another PR.

Do you think it is a regression of this PR we should address?

Why has tested it, except the author?

I don't think I understood your question.

Do you think it is a regression of this PR we should address?

The PR does not address changes to method upgradeVirtualMachine; therefore, the topic you are questioning would not be a regression of this PR.

Why has tested it, except the author?

I don't think I understood your question.

Sorry typo.
Who has tested it, except the author?

Do you think it is a regression of this PR we should address?

The PR does not address changes to method upgradeVirtualMachine; therefore, the topic you are questioning would not be a regression of this PR.

please ignore my question regarding the vm details, They are not saved in database before upgrade, so no need to restore.

Who has tested it, except the author?

I tested it; I do not know if anybody else has tested it too.

please ignore my question regarding the vm details, They are not saved in database before upgrade, so no need to restore.

Yes. The only thing this PR does is facilitate calling the API. For instance, without this PR, if one has a VM with 1 vCPU and 1 GB RAM and wants to scale it to 2 GB RAM, one has to call the API with both the current vCPU count and the new RAM size. With this PR, one will only have to pass the new RAM size in the parameters; ACS will retrieve the current VM's vCPU count and will add it to the details map to be updated; thus, passing the current vCPU count and the new RAM size will have the same behavior as just passing the new RAM size. It just updates the details map (if necessary) before scaling the VM. This applies to RAM, frequency and vCPU.

I created PR #8673 to improve the methods names and docs.

…#8234) * allow change only one parameter during live scale * Update server/src/main/java/com/cloud/vm/UserVmManagerImpl.java Co-authored-by: sato03 <henriquesato2003@gmail.com> * apply change method name * Update server/src/main/java/com/cloud/vm/UserVmManagerImpl.java Co-authored-by: João Jandre <48719461+JoaoJandre@users.noreply.github.com> --------- Co-authored-by: Gabriel <gabriel.fernandes@scclouds.com.br> Co-authored-by: sato03 <henriquesato2003@gmail.com> Co-authored-by: João Jandre <48719461+JoaoJandre@users.noreply.github.com>

hsato03 reviewed Nov 14, 2023

View reviewed changes

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java Outdated Show resolved Hide resolved

DaanHoogland approved these changes Nov 15, 2023

View reviewed changes

JoaoJandre reviewed Nov 16, 2023

View reviewed changes

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java Outdated Show resolved Hide resolved

JoaoJandre approved these changes Nov 17, 2023

View reviewed changes

hsato03 approved these changes Nov 28, 2023

View reviewed changes

DaanHoogland added this to the 4.19.0.0 milestone Nov 29, 2023

borisstoyanov self-assigned this Nov 29, 2023

github-actions bot added the status:has-conflicts label Dec 7, 2023

GaOrtiga force-pushed the allow_change_one_parameter_on_live_scale branch from b9759d0 to 1de263a Compare December 7, 2023 13:08

github-actions bot removed the status:has-conflicts label Dec 7, 2023

DaanHoogland added the status:needs-testing label Dec 12, 2023

shwstppr removed this from the 4.19.0.0 milestone Dec 14, 2023

Gabriel and others added 3 commits February 6, 2024 13:56

allow change only one parameter during live scale

83af447

Update server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

e817a59

Co-authored-by: sato03 <henriquesato2003@gmail.com>

apply change method name

d05f25f

GaOrtiga force-pushed the allow_change_one_parameter_on_live_scale branch from 1de263a to d05f25f Compare February 6, 2024 16:57

github-actions bot removed the status:has-conflicts label Feb 6, 2024

JoaoJandre reviewed Feb 8, 2024

View reviewed changes

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java Outdated Show resolved Hide resolved

Update server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

17f64f4

Co-authored-by: João Jandre <48719461+JoaoJandre@users.noreply.github.com>

GutoVeronezi merged commit a31449b into apache:main Feb 14, 2024

DaanHoogland modified the milestones: 4.19.1.0, 4.20.0.0 Feb 15, 2024

weizhouapache reviewed Feb 16, 2024

View reviewed changes

DaanHoogland removed the status:needs-testing label Jan 28, 2026


		updateInstanceDetails(cmdDetails, vm, newServiceOfferingId);

		boolean result = upgradeVirtualMachine(vmId, newServiceOfferingId, cmdDetails);

Conversation

GaOrtiga commented Nov 14, 2023

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

How Has This Been Tested?

Uh oh!

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DaanHoogland commented Nov 16, 2023

Uh oh!

blueorangutan commented Nov 16, 2023

Uh oh!

blueorangutan commented Nov 16, 2023

Uh oh!

DaanHoogland commented Nov 16, 2023

Uh oh!

blueorangutan commented Nov 16, 2023

Uh oh!

Uh oh!

blueorangutan commented Nov 17, 2023

Uh oh!

JoaoJandre left a comment

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi commented Nov 28, 2023

Uh oh!

blueorangutan commented Nov 28, 2023

Uh oh!

hsato03 left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Nov 28, 2023

Uh oh!

shwstppr commented Dec 6, 2023

Uh oh!

github-actions bot commented Dec 7, 2023

Uh oh!

shwstppr commented Dec 8, 2023

Uh oh!

blueorangutan commented Dec 8, 2023

Uh oh!

blueorangutan commented Dec 8, 2023

Uh oh!

DaanHoogland commented Dec 8, 2023

Uh oh!

blueorangutan commented Dec 8, 2023

Uh oh!

blueorangutan commented Dec 9, 2023

Uh oh!

github-actions bot commented Feb 5, 2024

Uh oh!

DaanHoogland commented Feb 6, 2024

Uh oh!

blueorangutan commented Feb 6, 2024

Uh oh!

blueorangutan commented Feb 6, 2024

Uh oh!

DaanHoogland commented Feb 7, 2024

Uh oh!

blueorangutan commented Feb 7, 2024

Uh oh!

blueorangutan commented Feb 8, 2024

Uh oh!

Uh oh!

DaanHoogland commented Feb 9, 2024

Uh oh!

DaanHoogland commented Feb 12, 2024

Uh oh!

blueorangutan commented Feb 12, 2024

Uh oh!

blueorangutan commented Feb 12, 2024

Uh oh!

codecov bot commented Nov 15, 2023 •

edited

Loading

weizhouapache Feb 16, 2024 •

edited

Loading